-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSS-4838 Audit Log improvements #1042
CSS-4838 Audit Log improvements #1042
Conversation
ff9d3fa
to
758e91a
Compare
internal/rpc/proxy.go
Outdated
@@ -17,6 +17,7 @@ import ( | |||
"github.com/canonical/jimm/internal/auth" | |||
"github.com/canonical/jimm/internal/dbmodel" | |||
"github.com/canonical/jimm/internal/errors" | |||
"github.com/canonical/jimm/internal/jimm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, not sure how I feel about bring internal/jimm as a dependency in this package, it's only for the jimm.NewConversationID() function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the NewConversationID into a utils package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use juju's new convo id func? If it's exported*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not exported https://github.com/juju/juju/blob/3.2/core/auditlog/auditlog.go#L176 (it's a very simple function that doesn't need to change when Juju changes, so I figured it's okay in this case to just copy-paste it)
I'll move it into a utils package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
internal/rpc/proxy.go
Outdated
@@ -17,6 +17,7 @@ import ( | |||
"github.com/canonical/jimm/internal/auth" | |||
"github.com/canonical/jimm/internal/dbmodel" | |||
"github.com/canonical/jimm/internal/errors" | |||
"github.com/canonical/jimm/internal/jimm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use juju's new convo id func? If it's exported*
- Created new utils package
Description
While writing the audit log doc canonical/jaas-documentation#21, it was found that several improvements could be made and some bugs fixed.
omitempty
to various audit log fields to make the returned list cleareroffset
field was never set.Engineering checklist
Check only items that apply